[ClangImporter] Import constant values for 'const' globals #80749
Conversation
kubamracek
requested review from
jckarter,
beccadax,
DougGregor,
artemcm and
tbkka
kubamracek
requested review from
zoecarver,
egorzhdan,
ian-twilightcoder,
Xazax-hun,
j-hui,
fahadnayyar and
susmonteiro
as code owners
| static const bool static_const_bool = true; | ||
| static const char static_const_char = 42; | ||
| static const long static_const_long = 42; | ||
| static const float static_const_float = 42.0; | ||
| static const double static_const_double = 42.0; |
There was a problem hiding this comment.
Some more edge cases it might be good to test:
- Literal uses, say,
ullto specify a different type from the constant. (Trying to make sure we don't infer a type from the literal like we might in a macro.) - Literal is too big for the type of the constant it's being assigned to (e.g. a
charinitialized to500). - Initializer refers to another constant which clang could potentially evaluate to find the value.
- Initializer refers to another constant which clang cannot evaluate.
There was a problem hiding this comment.
Maybe not that interesting, but I think it might be nice to test some C++-y things as well:
- Constexpr globals (that potentially are computed by constexpr functions)
- Constants in a namespace (as namespaces are imported as enums)
- Constants in class scope (static and non-static), even if we currently do not import them
There was a problem hiding this comment.
👍 will add all these tests, I started gathering the list of TODOs in the PR description
There was a problem hiding this comment.
I think this should now all be covered by tests.
|
@beccadax thanks for the awesome quick review, will add the coverage and find out the answers! :) |
lib/ClangImporter/ImportDecl.cpp
Outdated
| decl->getType(), ImportTypeKind::Value, | ||
| ImportDiagnosticAdder(Impl, decl, decl->getLocation()), | ||
| isInSystemModule(dc), Bridgeability::None, ImportTypeAttrs()); | ||
| result = synthesizer.createConstant( |
There was a problem hiding this comment.
I see that we bail for static constexpr variables above. I wonder if importing them as a constant would work for the supported types.
There was a problem hiding this comment.
Not for static constexpr, but for constexpr that are imported as static, i.e. they are not top-level (in a namespace, or in a class). I quickly tried just removing this restriction, and some basic stuff imports fine, so I don't see a reason why we need this restriction.
That said, I'd like to leave this as a follow-up and a separate change, so that I can explore the consequences here a bit more. Maybe @beccadax has some background on why this restriction exists in the first place?
There was a problem hiding this comment.
Actually, this provides the reasoning: #60154
Though it's light on the details...
| static const bool static_const_bool = true; | ||
| static const char static_const_char = 42; | ||
| static const long static_const_long = 42; | ||
| static const float static_const_float = 42.0; | ||
| static const double static_const_double = 42.0; |
There was a problem hiding this comment.
Maybe not that interesting, but I think it might be nice to test some C++-y things as well:
- Constexpr globals (that potentially are computed by constexpr functions)
- Constants in a namespace (as namespaces are imported as enums)
- Constants in class scope (static and non-static), even if we currently do not import them
Importing |
|
@swift-ci please test |
…swift + ClangImporter/const_values_cxx.swift
…adjust C++ lit tests
kubamracek
force-pushed
the
clangimport-const-values
branch
from
3dbc498 to
8e6071e
Compare
…s when importing const values
|
@swift-ci please test |
|
@swift-ci Please test compiler performance |
…t to fix Windows CI test failure
|
@swift-ci please test |
|
@swift-ci Please benchmark |
|
@swift-ci please test Linux platform |
|
@swift-ci please test Linux platform |
1 similar comment
|
@swift-ci please test Linux platform |
|
@swift-ci Please benchmark |
|
Thanks everyone for the help and reviews! Merging! :) |
Macro-defined constants in C (
#define FOO 42) get the concrete value imported (as a synthesized getter) into Swift. For global variables with initializer values (in bridging headers and in .h files in modules), we today only import them as external declarations. This PR starts importing the values of constant integer and floating-point global variables. Motivation is two-fold:@constand constant-value evaluation (see [Compile Time Values] Add a new experimental feature and the parsing of the@constattribute #79753, [Compile Time Values] Implement a mandatory SIL pass to verify '@const' values #79290 and https://forums.swift.org/t/pitch-3-swift-compile-time-values/77434), where it would be very useful to be able to use imported constants from C in Swift constant expressions.TODOs in this PR:
__attribute__((swift_name))to import as a memberullsuffix)TODOs leaving as follow-up: